Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

✨(mailboxes) link mailbox creation to dimail-api #261

Merged
merged 4 commits into from
Aug 9, 2024
Merged

✨(mailboxes) link mailbox creation to dimail-api #261

merged 4 commits into from
Aug 9, 2024

Conversation

mjeammet
Copy link
Member

@mjeammet mjeammet commented Jun 21, 2024 •
edited
Loading

Purpose

We want to link la Régie to the API provising our OpenExchange server (dimail-api)

Proposal

  • add a field "secret" on domain, to check credentials in the provisioning API
  • create a client to get headers and send request to email provisioning API
  • modify "save" method in mailbox object to call client upon email creation

@mjeammet mjeammet changed the title ✨(mailboxes) add webhook to dimail-api ✨(mailboxes) link mailbox creation to dimail-api Jun 28, 2024
@mjeammet mjeammet force-pushed the mpj/add-dimail-api-webhook branch 2 times, most recently from 309725b to 635a9d7 Compare July 16, 2024 16:56
@mjeammet mjeammet linked an issue Jul 22, 2024 that may be closed by this pull request
Create mailboxes from Régie (via dimail-api) #229
Closed
@mjeammet mjeammet added the WIP label Jul 31, 2024
@mjeammet mjeammet self-assigned this Jul 31, 2024
@mjeammet mjeammet force-pushed the mpj/add-dimail-api-webhook branch 9 times, most recently from affcaf6 to 1e34d32 Compare August 7, 2024 13:43
mjeammet
mjeammet commented Aug 7, 2024
View reviewed changes
src/backend/mailbox_manager/models.py Outdated
total=4,
backoff_factor=0.1,
status_forcelist=[500, 502],
allowed_methods=["PATCH"],
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why PATCH? Because of idempotency? Shouldn't we retry on a "GET" too?

Also, why would you retry on a 500?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's now in the utils folder but the question remains whole.

@mjeammet mjeammet force-pushed the mpj/add-dimail-api-webhook branch 4 times, most recently from c03a748 to 26924bb Compare August 8, 2024 15:08
@sdemagny sdemagny force-pushed the mpj/add-dimail-api-webhook branch 3 times, most recently from c4a3b50 to 0f74a64 Compare August 8, 2024 22:03
@sdemagny sdemagny marked this pull request as ready for review August 8, 2024 22:13
@sdemagny sdemagny force-pushed the mpj/add-dimail-api-webhook branch 4 times, most recently from ac18d1e to 2c2a921 Compare August 8, 2024 22:49
sdemagny
sdemagny reviewed Aug 8, 2024
View reviewed changes
src/backend/mailbox_manager/utils/dimail.py Outdated
if response.status_code == 201:
extra = {
"response": response.content.decode("utf-8")
} # Log mailbox info (including password !)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

on log le password ? c'est pas une bonne chose ça non ?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

En fait, je sais pas quoi faire de ce password pour l'instant. On avait mentionné que ce serait OX qui l'envoyait mais on s'est pas encore capté pour définir la brique et potentiellement la faire avec eux.

Comme je suis pas trop sûre de comment on envoie des mails et de si on est en capacité d'envoyer des mails avec la Régie maintenant, j'ai fait ce hack dégueu pour les démos mais j'avoue c'est pas trop prod-ready.

@sdemagny
Copy link
Collaborator

sdemagny commented Aug 9, 2024

Il va rester cette histoire de config de MAIL_PROVISIONING_API_URL à voir avec Anto si il peut.

J'ai rajouté un commit avec une gestion de la 403 retournée par l'API dimail en cas de mauvaise conf du secret sur le nom de domaine.

Ce qu'elle contient :

  • manage 403 returned by dimail API when mail domain secret is not valid
  • improve some tests
  • improve MailboxFactory adding a RequestsMock success for dimail API POST call
  • override 403.html to give a nice display for mailbox creation failed through django admin
  • an error message is displayed on mailbox creation form of frontend

Aperçu du rendu de l'erreur 403 renvoyé par dimail dans l'interface d'admin et sur le front:

Coté admin django:
Capture d’écran 2024-08-09 à 02 14 41
--> Ya mieux à faire pour l'interface d'admin mais ya zero raison que j'y passe plus de temps puisque c'est pas un chemin de création de boite mail normalement.

Coté frontend:
Capture d’écran 2024-08-09 à 02 13 53

sdemagny
sdemagny reviewed Aug 9, 2024
View reviewed changes
src/backend/mailbox_manager/factories.py
@classmethod
def _create(cls, model_class, *args, use_mock=True, **kwargs):
domain = kwargs["domain"]
if use_mock and isinstance(domain, models.MailDomain):
Copy link
Collaborator

@sdemagny sdemagny Aug 9, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

C'est un peu étrange ce que je suis obligée de faire pour que les tests qui concernent la création de boite mail sans mail domaine fonctionnent. Ya un truc qui me chiffonne avec ces tests. À discuter tranquillement.
@mjeammet

sdemagny
sdemagny approved these changes Aug 9, 2024
View reviewed changes
Copy link
Collaborator

@sdemagny sdemagny left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

YOLO !!!!

mjeammet and others added 4 commits August 9, 2024 13:25
@mjeammet
🗃️(domains) add "secret" field to domains
4cb19b5 
add a "secret" field to domain model. This secret will be used as
password in mail provisioning API.
@mjeammet
✨(mailboxes) add mail provisioning api integration
a01afc1 
We want people to create new mailboxes in La Régie.
This commit adds integration with intermediary dimail-api,
which will in turn send our email creation request to Open-Xchange.
@mjeammet
✅(test) add test list mailboxes non existing domain
bf1b0e8 
Test that API raises a 404 when trying to list mailboxes
of a domain that does not exist.
@sdemagny @mjeammet
✨(mailboxes) manage bad secret sent to dimail API
a20eb99 
- manage 403 returned by dimail API when mail domain secret is not valid
- improve some tests
- improve MailboxFactory to mock success for dimail API POST call
- override 403.html to return a nice failing error in django admin
- an error message is displayed on mailbox creation form of frontend
@mjeammet mjeammet merged commit a7a923e into main Aug 9, 2024
15 checks passed
@mjeammet mjeammet deleted the mpj/add-dimail-api-webhook branch August 9, 2024 11:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@AntoLC AntoLC AntoLC left review comments

@sdemagny sdemagny sdemagny approved these changes

Assignees

@mjeammet mjeammet

Labels
backend noChangeLog WIP
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Create mailboxes from Régie (via dimail-api)
3 participants
@mjeammet @sdemagny @AntoLC